Document evaluation results table with pagination #300

andresgutgon · 2024-09-27T13:00:55Z

What?

Paginate document evaluation results and refactor pagination Refactor pagination to make it more generic and easy to use in any table we want to add pagination

Issue

#283

TODO

Generic paginateQuery method
Fix document logs table after refactor
Fix types for augmented query select
Fix wip work on refactoring lib/pagination/* I left this broken. Start here
Paginate evaluation results table
New styles for pagination in table bottom
Evaluation ID result needs to be a page not a layout because NextJS doesn't receive searchParams in layouts
Log detail in /logs and evaluation results need to be scrollable

Next PR

Add tests to paginateQuery
DRY sidebar document info and use the right design as commented here

apps/web/src/actions/commits/fetchCommitsByProjectAction.ts

andresgutgon · 2024-09-27T18:52:43Z

apps/web/src/actions/commits/fetchCommitsByProjectAction.ts

+          project: ctx.project,
+          filterByStatus: input.status,
+        })
+        .$dynamic(),


No so bad in the end. We just need to invoke $dynamic() in our queries and it will be paginated and fully typed

andresgutgon · 2024-09-27T18:56:19Z

packages/core/src/repositories/repository.ts

-    const rows = await query.limit(pageSize).offset((page - 1) * pageSize)
-    const count = rows[0]?.__count ? Number(rows[0]?.__count) : 0
-    return [rows, count]
-  }


Repositiries are not the right place to put pagination concerns. Now one doubt I have is with the generic Repository.findAll(). Now it executes the query. I think would be better to return the query and use the paginateQuery service to let it do the pagination work. I wont do this refactor here

But what do you think?

hmm i think it's weird ideally you want the repository.findall method to return the results not the query, any way we can integrate the pagination into the findall method?

pagination is page aware in the sense that you have to pass a base URL to generate
< prev and next > URLs. We can pass this prop to the findAll but it feels weird to me. For what I saw in Rails word pagination is aware of ORM but ORM is not aware of pagination.

No big deal whatever you prefer

hmm ya, well then yes i would remove the findall method from repositories all together and use the scope plus the new pagination method you created

andresgutgon · 2024-09-27T18:56:56Z

packages/core/src/services/documentLogs/computeDocumentLogsWithMetadata.test.ts

@@ -3,7 +3,7 @@ import { describe, expect, it } from 'vitest'
 import { mergeCommit } from '../../services/commits'
 import { updateDocument } from '../../services/documents'
 import * as factories from '../../tests/factories'
-import { computeDocumentLogsWithMetadata } from './computeDocumentLogsWithMetadata'
+import { computeDocumentLogsWithMetadataQuery } from './computeDocumentLogsWithMetadata'


By convention if a query service return the lazy query call it [something]Query so we know

andresgutgon · 2024-09-28T17:12:30Z

...uid]/evaluations/[evaluationId]/_components/EvaluationResults/EvaluationResultInfo/index.tsx

-          onSelect={setSelectedTab}
-        />
-        <div className='flex flex-col relative w-full h-full max-h-full max-w-full overflow-auto'>
+      <div


All this wrapper classes are supper similar between

Evaluation log info side panel

Evaluation original log modal panel

Log sidebar info panel

Probably is there an abstraction. For now I'll leave un-DRY

now that you are at it maybe we can implement the correct design for the original log design

andresgutgon · 2024-09-28T17:13:29Z

...ommitUuid]/documents/[documentUuid]/evaluations/[evaluationId]/_lib/fetchEvaluationCached.ts

+  const { workspace } = await getCurrentUser()
+  const evaluationScope = new EvaluationsRepository(workspace.id)
+  return evaluationScope.find(id).then((r) => r.unwrap())
+})


Now that evaluation is fetched in layout and page is a perfect use case for cache

andresgutgon · 2024-09-28T17:18:33Z

.../documents/[documentUuid]/evaluations/[evaluationId]/_components/EvaluationResults/index.tsx

  const { data: evaluationResults, mutate } = useEvaluationResultsWithMetadata(
    {
      evaluationId: evaluation.id,
      documentUuid: document.documentUuid,
      commitUuid: commit.uuid,
      projectId: project.id,
+      page,
+      pageSize,


My guess was that because a different page means a different URL all the client is wipe out. But that's not the case and when you move to page 2 you still see cached page 1 in the client. So I did that the store also is aware of pagination in the cache key so whenever change it make a new fetch

This will need to be taken into account when we introduce filters in evaluation results table

andresgutgon · 2024-09-28T17:20:43Z

...rojectId]/versions/[commitUuid]/documents/[documentUuid]/evaluations/[evaluationId]/page.tsx

+    documentUuid: string
+    evaluationId: string
+  }
+  searchParams: QueryParams


So before this changes everthing was in the layout and create batch modal was route URL. This is not possible because query params (search params) are not revalidated in layouts so I need to convert results table in a page

More info:
https://nextjs.org/docs/app/api-reference/file-conventions/layout#layouts-do-not-receive-searchparams

classic question: is this messing with any modals in subroutes?

Yes, create batch was a route and now is not because if we keep as route the table is not visible when open

andresgutgon · 2024-09-28T17:21:42Z

...rojectId]/versions/[commitUuid]/documents/[documentUuid]/evaluations/[evaluationId]/page.tsx

+      documentUuid: params.documentUuid,
+      draft: commit,
+    }).$dynamic(),
+  })


I'm really happy with the final API of paginateQuery it does 2 things:

Paginate the SQL query

Generates a IPagination object to be consumed by pagination UI components

andresgutgon · 2024-09-28T17:22:30Z

...commitUuid]/documents/[documentUuid]/logs/_components/DocumentLogs/DocumentLogInfo/index.tsx

 }) {
  const [selectedTab, setSelectedTab] = useState<string>('metadata')
+  const ref = useRef<HTMLDivElement>(null)
+  const height = useDynamicHeight({ ref, paddingBottom: 16 })


Oh man, web apps with scrollable areas are not as easy as doing web pages. I don't care what DHH says

andresgutgon · 2024-09-28T17:24:52Z

packages/web-ui/src/ds/molecules/Chat/MessageList/index.tsx

-          </div>
-        </div>
-      )}
-      {visibleMessages.map((message, index) => (


Hiding messages is gone cc @cesr

why didn't we like this? in evaluations it's useless to see a ultra long conversation chain in a width of 150 pixels

Refactor pagination to make it more generic and easy to use in any table we want to add pagination

andresgutgon · 2024-09-28T17:40:54Z

packages/core/src/repositories/documentVersionsRepository/index.test.ts

@@ -47,7 +47,8 @@ describe('DocumentVersionsRepository', () => {

      const documents = result.unwrap()
      expect(documents).toHaveLength(2)
-      expect(documents.map((d) => d.path)).toEqual(['foo', 'bar'])
+      const paths = documents.map((d) => d.path).sort()
+      expect(paths).toEqual(['bar', 'foo'])


Troll test failing in my branch. I didn't touch this

.../[commitUuid]/documents/[documentUuid]/_components/DocumentEditor/Editor/Playground/Chat.tsx

packages/core/src/lib/pagination/paginate.ts

geclos · 2024-09-29T09:32:37Z

...sions/[commitUuid]/documents/[documentUuid]/evaluations/[evaluationId]/create-batch/page.tsx

where is this route now?

Inside ./Actions/**.tsx component. Now is not a route

Like wat? We have been here before

well it's a pretty uncommon modal so ok this time,but please let's not make it a common thing that we remove modal from routes and into components

Also is this so important? What we lost in this specific case?

that's why i said this

well it's a pretty uncommon modal so ok this time

not super important in this case. But since you ask we lose the ability to directly link to the creation modal if we ever want to.

When I wrote my message yours was not in the UI. Lo del real time

andresgutgon added the 🚧 wip Work in progress label Sep 27, 2024

andresgutgon force-pushed the feature/paginate-document-evaluation-results-table branch 2 times, most recently from cc39e83 to 1a85bbc Compare September 27, 2024 16:02

andresgutgon commented Sep 27, 2024

View reviewed changes

apps/web/src/actions/commits/fetchCommitsByProjectAction.ts Outdated Show resolved Hide resolved

andresgutgon commented Sep 27, 2024

View reviewed changes

andresgutgon force-pushed the feature/paginate-document-evaluation-results-table branch from 2374049 to 1bba8fc Compare September 28, 2024 17:10

andresgutgon commented Sep 28, 2024

View reviewed changes

andresgutgon force-pushed the feature/paginate-document-evaluation-results-table branch from 1bba8fc to 67f6f25 Compare September 28, 2024 17:16

andresgutgon commented Sep 28, 2024

View reviewed changes

andresgutgon removed the 🚧 wip Work in progress label Sep 28, 2024

Paginate document evaluation results and refactor pagination

2e875a1

Refactor pagination to make it more generic and easy to use in any table we want to add pagination

andresgutgon force-pushed the feature/paginate-document-evaluation-results-table branch from 67f6f25 to 2e875a1 Compare September 28, 2024 17:29

Fix troll test

20c4b1a

andresgutgon commented Sep 28, 2024

View reviewed changes

geclos reviewed Sep 29, 2024

View reviewed changes

.../[commitUuid]/documents/[documentUuid]/_components/DocumentEditor/Editor/Playground/Chat.tsx Show resolved Hide resolved

geclos reviewed Sep 29, 2024

View reviewed changes

packages/core/src/lib/pagination/paginate.ts Show resolved Hide resolved

geclos reviewed Sep 29, 2024

View reviewed changes

andresgutgon merged commit b29a724 into main Sep 30, 2024
4 checks passed

andresgutgon deleted the feature/paginate-document-evaluation-results-table branch September 30, 2024 06:59

github-actions bot locked and limited conversation to collaborators Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document evaluation results table with pagination #300

Document evaluation results table with pagination #300

andresgutgon commented Sep 27, 2024 •

edited

Loading

andresgutgon Sep 27, 2024

andresgutgon Sep 27, 2024

geclos Sep 29, 2024

andresgutgon Sep 29, 2024

geclos Sep 29, 2024

andresgutgon Sep 27, 2024 •

edited

Loading

andresgutgon Sep 28, 2024

geclos Sep 28, 2024

andresgutgon Sep 28, 2024

andresgutgon Sep 28, 2024

andresgutgon Sep 28, 2024

andresgutgon Sep 28, 2024

geclos Sep 29, 2024

andresgutgon Sep 29, 2024

andresgutgon Sep 28, 2024

andresgutgon Sep 28, 2024

andresgutgon Sep 28, 2024

geclos Sep 28, 2024

andresgutgon Sep 28, 2024

geclos Sep 29, 2024

andresgutgon Sep 29, 2024

geclos Sep 29, 2024

andresgutgon Sep 29, 2024

geclos Sep 29, 2024 •

edited

Loading

andresgutgon Sep 29, 2024

geclos Sep 29, 2024

andresgutgon Sep 29, 2024

Document evaluation results table with pagination #300

Document evaluation results table with pagination #300

Conversation

andresgutgon commented Sep 27, 2024 • edited Loading

What?

Issue

TODO

Next PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresgutgon Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geclos Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresgutgon commented Sep 27, 2024 •

edited

Loading

andresgutgon Sep 27, 2024 •

edited

Loading

geclos Sep 29, 2024 •

edited

Loading